Skip to content

fix: address PR 162 security regression blockers#163

Open
helizaga wants to merge 8 commits intomainfrom
tommy/pr-162-fixes
Open

fix: address PR 162 security regression blockers#163
helizaga wants to merge 8 commits intomainfrom
tommy/pr-162-fixes

Conversation

@helizaga
Copy link
Copy Markdown
Collaborator

@helizaga helizaga commented Apr 3, 2026

This supersedes #162 because the original PR branch cannot be updated by maintainers (maintainerCanModify: false).

It keeps the intended hardening from #162, fixes the security and regression blockers found during review, and includes a final cleanup pass to reduce long-term Bash maintenance risk without changing user-facing behavior.

What changed:

  • align trust hashing between git gtr trust and generated init wrappers
  • scope hook trust markers by repository path plus hook definitions, so identical hook text in different repos is not implicitly trusted
  • persist the reviewed trust snapshot and fail git gtr trust if hooks change during confirmation
  • write trust markers atomically and validate their stored payload before treating hooks as approved
  • invalidate cached shell init output for the trust-flow changes
  • resolve repo-level .gtrconfig correctly from worktrees in generated wrappers
  • reject generic adapter filesystem paths like ./tool
  • reject generic shell/interpreter wrapper commands like sh -c ...
  • replace generic adapter command parsing with a non-eval parser that preserves quoted arguments safely
  • preserve configured flags for override-backed adapters such as claude --continue, cursor --resume, and nano -w
  • restore documented exclude patterns like node_modules/* and */.*
  • fix terminal spawning so multi-word commands are passed correctly
  • make cmd_trust fail when trust persistence fails
  • keep wrapper completions in sync, including Fish support for trust
  • single-source the Bash/Zsh init trust helpers so shell wrapper trust logic cannot drift as easily
  • simplify adapter parsing, validation, and generic execution flow so parsed argv is owned explicitly and not shared through a mutable global
  • add regression coverage for all of the above

Validation:

  • bats tests/
  • shellcheck bin/gtr bin/git-gtr lib/*.sh lib/commands/*.sh adapters/editor/*.sh adapters/ai/*.sh
  • ./scripts/generate-completions.sh --check

Original replacement-PR fix commit: 8bbf922.
Follow-up hardening commits: f2b7249, 825820e, e63c8a9, 7b90ec6.
Cleanup commit: 01d23d3.

Summary by CodeRabbit

  • New Features

    • Added git gtr trust to review/approve repository .gtrconfig hooks before they run; trust is stored per-repo+content and requires re-approval on change.
  • Security & Bug Fixes

    • Removed eval-based command execution and added safer command parsing; reject unsafe generic defaults and prevent accidental .git deletion.
    • Tightened terminal spawn behavior to avoid unintended shell evaluation.
  • Improvements

    • Updated shell completions (Bash/Zsh/Fish) and clarified help/config docs about safe adapter defaults.
    • GitHub workflows now use explicit permissions and pinned action commits.
  • Tests

    • Added extensive tests for trust flow, adapter parsing, copy safety, init output, and platform handling.

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 3, 2026

Walkthrough

Adds a new git gtr trust command to review/approve .gtrconfig hooks (stored per-content-hash), gates .gtrconfig hooks behind trust markers, replaces unsafe eval usages with safer parsing/execution, hardens adapter handling and copy safety, updates shell init/completions/tests, and pins GitHub Actions.

Changes

Cohort / File(s) Summary
GitHub Actions
\.github/workflows/homebrew.yml, \.github/workflows/lint.yml
Added top-level permissions: read-all; pinned actions (mislav/bump-homebrew-formula-action, actions/checkout) to specific commit SHAs.
CLI entry, docs & help
bin/git-gtr, README.md, lib/commands/help.sh
Added trust subcommand dispatch, README section documenting git gtr trust and trust lifecycle, and _help_trust() help text.
Shell completions
completions/_git-gtr, completions/git-gtr.fish, completions/gtr.bash, scripts/generate-completions.sh
Added trust to Zsh/Bash/Fish completion outputs and generation script.
Trust core & init integration
lib/commands/trust.sh, lib/hooks.sh, lib/commands/init.sh
New cmd_trust; SHA‑256 hook hashing, repo-scoped trust key, trust markers under XDG config, helpers to check/write trust, gating .gtrconfig hooks at runtime and in generated shell integration; bumped init cache schema.
Adapter parsing & execution safety
lib/adapters.sh, adapters/.../*.sh
Removed eval usage, added _parse_configured_command, _run_configured_command, safety validators; expose GTR_*_CMD_ARGS arrays and propagate configured args into adapters; override sourcing only for non-path selectors.
Environment & UI safety
lib/config.sh, lib/ui.sh, lib/args.sh
Replaced eval env fallback with printenv, switched eval variable assignment to printf -v, and simplified IFS handling in _pa_match_flag.
Copy & filesystem safety
lib/copy.sh
Reject overly-broad exclude patterns, skip excludes targeting .git metadata, and prevent removal of .git or its contents.
Platform / terminal spawn
lib/platform.sh
Refined terminal spawn wrapper: set SHELL_AFTER, handle empty vs non-empty commands, and use bash -lc when executing commands so multi-word commands are preserved.
Tests
tests/*.bats, tests/*
Added/updated Bats tests: adapter parsing/override behavior, _apply_directory_excludes safety, hook hashing/trust logic, cmd_trust flows (success, failure, concurrent-change handling), init caching and generated wrapper checks, and platform spawn argument handling.

Sequence Diagram(s)

sequenceDiagram
    participant U as User
    participant CLI as git gtr (CLI)
    participant Git as git (config/.gtrconfig)
    participant Trust as TrustStore\n(XDG_CONFIG_HOME)
    participant Hooks as Hook Runner

    rect rgba(200,230,255,0.5)
    U->>CLI: git gtr trust
    CLI->>Git: locate `.gtrconfig` and read `hooks.*`
    end

    rect rgba(220,255,200,0.5)
    CLI->>CLI: compute SHA‑256 of hooks content
    CLI->>Trust: check for existing trust marker
    Trust-->>CLI: marker present? (yes/no)
    end

    alt not trusted
      CLI->>U: display hook contents + prompt_yes_no
      U-->>CLI: confirm
      CLI->>Trust: write trust marker for content hash
      Trust-->>CLI: success / failure
      CLI->>U: exit 0 on success / exit 1 on failure
    else already trusted
      CLI->>U: report hooks already trusted
    end

    rect rgba(255,240,200,0.5)
    Note right of Hooks: Later hook execution (e.g., run_hooks)\nreads hooks via Trust-aware helper
    Hooks->>Trust: request hooks for phase
    Trust-->>Hooks: return trusted `.gtrconfig` hooks + local `gtr.hook.*`
    Hooks->>Hooks: execute approved hooks
    end
Loading

Estimated code review effort

🎯 4 (Complex) | ⏱️ ~60 minutes

Poem

🐇 I hopped through configs, counted each hook’s art,

I hashed and I guarded — no shell-eval heart.
I skipped the .git and kept adapters tame,
Stamped trusted lines so hooks run by name.
A nibble, a marker — now the repo’s less game.

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 54.79% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'fix: address PR 162 security regression blockers' directly and specifically describes the main objective of the changeset: addressing security regressions from PR #162. It accurately reflects the primary purpose across the modified files.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch tommy/pr-162-fixes

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

Caution

Some comments are outside the diff and can’t be posted inline due to platform limitations.

⚠️ Outside diff range comments (1)
lib/adapters.sh (1)

307-360: ⚠️ Potential issue | 🟠 Major

Reject path separators in adapter names to prevent local command execution.

The metacharacter filter (lines 354–360) blocks shell operators but not slashes, so relative paths like ./tool and ../tool pass through. Since command -v accepts relative paths and the full name is later passed to eval in _run_configured_command(), a user-controlled config value (e.g., git gtr config set gtr.editor.default ./malicious) would execute arbitrary binaries from the current directory instead of only PATH-resolved commands. Reject / and \ in the first token, or validate that the resolved command is in PATH only.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/adapters.sh` around lines 307 - 360, The metacharacter check currently
blocks shell operators but allows path separators, letting relative paths like
"./tool" pass; update the validation in the adapter loader to reject path
separators for the configured command (check either "name" or the extracted
"cmd_name" for '/' and '\' characters) before accepting it, and return an error
like other checks; alternatively, resolve the command with "command -v" and
ensure the resolved path is a PATH-resolved command (not a relative/containment
path) before proceeding — reference the variables "name" and "cmd_name" and the
caller "_run_configured_command" when making this change.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/commands/init.sh`:
- Around line 275-277: The Fish wrapper's hardcoded subcommand list is out of
sync with the Bash list (the Bash block uses COMP_CWORD/COMPREPLY with compgen
and includes "trust"); update the Fish completion template in this file to match
the Bash subcommand list (add "trust" and any other missing entries) so that
running the init fish generator (git gtr init fish) produces the same
completions as the COMPREPLY/compgen block; alternatively, factor the subcommand
list into a single variable and reference it from both the COMP_CWORD/COMPREPLY
block and the Fish template so they stay synchronized.

In `@lib/commands/trust.sh`:
- Around line 13-40: cmd_trust reads and displays hooks then calls
_hooks_mark_trusted which re-hashes the file after the prompt; compute the
reviewed marker (e.g., call the hashing function/_get_marker used by
_hooks_mark_trusted) immediately after reading hook_content and before calling
prompt_yes_no, store that marker (and reviewed path if needed), show the hooks
as before, then after prompt_yes_no pass that stored marker into a new variant
of the marker-writer (or change _hooks_mark_trusted to accept a marker argument)
so you persist the exact reviewed marker instead of re-computing it
post-confirmation; keep _hooks_are_trusted usage unchanged but compare against
the stored marker where appropriate.

In `@lib/hooks.sh`:
- Around line 16-32: The trust marker key is currently content-only (via
_hooks_file_hash) which makes markers shared across repos; update
_hooks_trust_path to incorporate a repository-unique identifier (e.g.,
repository root path, remote URL, or git object like commit/slug) into the
computed key so the path becomes "$_GTR_TRUST_DIR/<repo-id>-<hash>" instead of
just "<hash>", and likewise update the wrapper generation in
lib/commands/init.sh to include the same repo identifier when creating trust
markers so markers are repo-scoped; keep using _hooks_file_hash for the content
part and prepend or append the chosen repo-id consistently.

---

Outside diff comments:
In `@lib/adapters.sh`:
- Around line 307-360: The metacharacter check currently blocks shell operators
but allows path separators, letting relative paths like "./tool" pass; update
the validation in the adapter loader to reject path separators for the
configured command (check either "name" or the extracted "cmd_name" for '/' and
'\' characters) before accepting it, and return an error like other checks;
alternatively, resolve the command with "command -v" and ensure the resolved
path is a PATH-resolved command (not a relative/containment path) before
proceeding — reference the variables "name" and "cmd_name" and the caller
"_run_configured_command" when making this change.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 28b78bd9-7e2a-429d-863e-69b3767fa1e4

📥 Commits

Reviewing files that changed from the base of the PR and between c765e62 and 8bbf922.

📒 Files selected for processing (24)
  • .github/workflows/homebrew.yml
  • .github/workflows/lint.yml
  • README.md
  • bin/git-gtr
  • completions/_git-gtr
  • completions/git-gtr.fish
  • completions/gtr.bash
  • lib/adapters.sh
  • lib/args.sh
  • lib/commands/help.sh
  • lib/commands/init.sh
  • lib/commands/trust.sh
  • lib/config.sh
  • lib/copy.sh
  • lib/hooks.sh
  • lib/platform.sh
  • lib/ui.sh
  • scripts/generate-completions.sh
  • tests/adapters.bats
  • tests/cmd_trust.bats
  • tests/copy_safety.bats
  • tests/hooks.bats
  • tests/init.bats
  • tests/platform.bats

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

♻️ Duplicate comments (1)
lib/hooks.sh (1)

22-29: ⚠️ Potential issue | 🟠 Major

Path scoping still leaves stale approvals across content changes.

Hashing repo_root + hook hash fixes the old cross-path reuse, but it still does not re-prompt when the same checkout path starts executing different repo-local code. A trusted ./scripts/bootstrap survives branch updates or a fresh clone into the same directory as long as .gtrconfig is unchanged, and run_hooks() will then execute a payload the user never reviewed. This trust key needs a repository-state component that changes with the checked-out content, not just the path and hook string.

Also applies to: 41-59

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/hooks.sh` around lines 22 - 29, The current hook trust key uses only
path+hook content, so a different commit checked out at the same path reuses
approvals; update _hooks_file_hash to incorporate repository state by including
a stable commit identifier (e.g., git rev-parse --verify HEAD) or a tree hash of
the working tree when present, then pass that combined string into
_hooks_content_hash so any change to checked-out content invalidates prior
approvals; adjust callers like run_hooks to continue using _hooks_file_hash
unchanged so the trust key now reflects both .gtrconfig hook entries and the
current repository commit/tree state.
🧹 Nitpick comments (1)
lib/hooks.sh (1)

41-79: Keep the trust-key recipe single-sourced.

lib/hooks.sh and the generated helper in lib/commands/init.sh:129-155 now need to stay byte-for-byte aligned again. Since the last regression came from those drifting, I'd strongly prefer generating the wrapper helper from one shared source or template instead of maintaining two manual implementations.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/hooks.sh` around lines 41 - 79, The trust-key logic is duplicated between
lib/hooks.sh (functions _hooks_trust_key, _hooks_trust_key_for_content,
_hooks_trust_path, _hooks_trust_path_for_content) and the generated helper in
lib/commands/init.sh; consolidate to a single source by moving the core
trust-key recipe into one canonical implementation (either a shared template
file or a single library function) and change the generator to emit a thin
wrapper that calls that canonical implementation so both runtime and generated
code remain byte-for-byte aligned; ensure the generator references the canonical
symbol name (e.g., the single source function) when producing the wrapper and
remove the duplicate manual implementations.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/adapters.sh`:
- Around line 307-318: The override-loading code currently sources the adapter
file using adapter_selector/adapter_file then returns early (return 0), which
drops the original full name (e.g., "nano -w") so flags never reach the generic
runner; change the flow to source the override but do NOT return
immediately—instead set a marker (e.g., ADAPTER_OVERRIDE_LOADED=1 or set
adapter_override_file="$adapter_file") and let the function continue so the full
name variable (the original name variable) is preserved and passed to the
generic runner; apply the same change for the second override branch that
mirrors this logic (the block that uses adapter_selector/adapter_file again).
- Around line 183-193: The validation allows shell builtins and wrapper commands
with flags which can be exploited because _run_configured_command uses eval "set
-- $command_string"; change the validation to use type -P instead of command -v
so only PATH executables (not builtins) are accepted, and add a check that the
command string contains no option flags (tokens starting with -) or disallowed
wrapper executables (e.g., sh, bash, eval, builtin) before allowing it to be
stored/used; update the validation logic that currently checks metacharacters
(refer to the validation function called before _run_configured_command) to
explicitly reject tokens beginning with '-' and to reject commands where type -P
returns empty or where the resolved command name matches a blacklist of shell
wrappers.

In `@lib/hooks.sh`:
- Around line 91-102: _hooks_write_trust_marker() currently creates the target
file path then writes to it, which can leave a partial/empty marker on failure;
change it to write atomically by creating a temp file in the same directory
(e.g. "${trust_path}.tmp"), write the payload to that temp file, flush it (fsync
the file) and then mv the temp into place, and optionally fsync the directory
(using _GTR_TRUST_DIR) to ensure durability; additionally, tighten
_hooks_are_trusted() to validate the stored payload (e.g. check file is
non-empty and matches the expected config_file or pattern) before treating it as
trusted so a truncated/empty marker cannot be accepted.

---

Duplicate comments:
In `@lib/hooks.sh`:
- Around line 22-29: The current hook trust key uses only path+hook content, so
a different commit checked out at the same path reuses approvals; update
_hooks_file_hash to incorporate repository state by including a stable commit
identifier (e.g., git rev-parse --verify HEAD) or a tree hash of the working
tree when present, then pass that combined string into _hooks_content_hash so
any change to checked-out content invalidates prior approvals; adjust callers
like run_hooks to continue using _hooks_file_hash unchanged so the trust key now
reflects both .gtrconfig hook entries and the current repository commit/tree
state.

---

Nitpick comments:
In `@lib/hooks.sh`:
- Around line 41-79: The trust-key logic is duplicated between lib/hooks.sh
(functions _hooks_trust_key, _hooks_trust_key_for_content, _hooks_trust_path,
_hooks_trust_path_for_content) and the generated helper in lib/commands/init.sh;
consolidate to a single source by moving the core trust-key recipe into one
canonical implementation (either a shared template file or a single library
function) and change the generator to emit a thin wrapper that calls that
canonical implementation so both runtime and generated code remain byte-for-byte
aligned; ensure the generator references the canonical symbol name (e.g., the
single source function) when producing the wrapper and remove the duplicate
manual implementations.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cc37536c-f564-4ebe-adc2-f74003eb714e

📥 Commits

Reviewing files that changed from the base of the PR and between 8bbf922 and f2b7249.

📒 Files selected for processing (8)
  • lib/adapters.sh
  • lib/commands/init.sh
  • lib/commands/trust.sh
  • lib/hooks.sh
  • tests/adapters.bats
  • tests/cmd_trust.bats
  • tests/hooks.bats
  • tests/init.bats
✅ Files skipped from review due to trivial changes (2)
  • tests/init.bats
  • lib/commands/trust.sh
🚧 Files skipped from review as they are similar to previous changes (4)
  • tests/hooks.bats
  • tests/adapters.bats
  • lib/commands/init.sh
  • tests/cmd_trust.bats

@helizaga
Copy link
Copy Markdown
Collaborator Author

helizaga commented Apr 4, 2026

@coderabbitai review

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 4, 2026

✅ Actions performed

Review triggered.

Note: CodeRabbit is an incremental review system and does not re-review already reviewed commits. This command is applicable only when automatic reviews are paused.

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/hooks.sh`:
- Around line 117-118: If _hooks_canonical_config_path fails after creating the
temp file, the temp file referenced by temp_path (created via mktemp
"$_GTR_TRUST_DIR/.trust.XXXXXX") is leaked; update the failure path so that when
canonical_config_file=$(_hooks_canonical_config_path "$config_file") returns
non-zero you remove the temp file (rm -f "$temp_path") before returning (or set
a local cleanup/trap that removes temp_path on early exit) to ensure no
temporary files are left behind.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 42772e88-3740-46a9-bd38-764b54ca23f5

📥 Commits

Reviewing files that changed from the base of the PR and between f2b7249 and e63c8a9.

📒 Files selected for processing (9)
  • adapters/ai/claude.sh
  • adapters/ai/cursor.sh
  • adapters/editor/nano.sh
  • lib/adapters.sh
  • lib/commands/init.sh
  • lib/hooks.sh
  • tests/adapters.bats
  • tests/hooks.bats
  • tests/init.bats
🚧 Files skipped from review as they are similar to previous changes (4)
  • tests/hooks.bats
  • tests/adapters.bats
  • lib/adapters.sh
  • tests/init.bats

Copy link
Copy Markdown

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 3

🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@lib/adapters.sh`:
- Around line 296-304: Remove the overly restrictive pre-parse validations:
delete the helper functions _configured_command_uses_path_arg and
_configured_command_has_safe_syntax and stop rejecting arguments like
"~/.toolrc" or raw metacharacters, and instead only validate the adapter command
name inside _load_adapter(); ensure _load_adapter() no longer calls those two
functions and only checks the command name token (leaving GTR_*_CMD_ARGS
handling to the adapter overrides) so argv-style execution via "$@" can accept
literal path-like args and metacharacters as safe data.

In `@lib/commands/init.sh`:
- Around line 174-180: The regex passed to string replace in the branch handling
linked worktrees is over-escaped ('/\\.git$') so Fish treats the backslash
literally; change the pattern to '/\.git$' (single backslash) in the string
replace -r call that operates on _gtr_git_common_dir so the trailing '/.git'
suffix is correctly stripped (i.e., update the expression string replace -r
'/\\.git$' '' -- "$_gtr_git_common_dir" to use '/\.git$' instead).

In `@lib/hooks.sh`:
- Around line 135-175: The code currently treats failures from
_hooks_current_trust_path as success (returning 0), merging “no hooks” with real
errors; change the error handling so that when _hooks_current_trust_path fails
it returns non-zero (fail closed). Specifically, in _hooks_are_trusted() replace
the trust_path assignment branch that uses "|| return 0" with "|| return 1" so
resolution errors mark hooks untrusted, and in _hooks_mark_trusted() do the same
(replace "trust_path=$(_hooks_current_trust_path \"$config_file\") || return 0"
with "|| return 1") so marker writes are aborted on resolution errors.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: cf8e3b93-1ea4-40b2-a342-d1d7f67142a8

📥 Commits

Reviewing files that changed from the base of the PR and between e63c8a9 and 01d23d3.

📒 Files selected for processing (8)
  • README.md
  • lib/adapters.sh
  • lib/commands/help.sh
  • lib/commands/init.sh
  • lib/commands/trust.sh
  • lib/hooks.sh
  • tests/adapters.bats
  • tests/init.bats
🚧 Files skipped from review as they are similar to previous changes (4)
  • README.md
  • lib/commands/help.sh
  • tests/init.bats
  • lib/commands/trust.sh

Comment on lines +296 to +304
_configured_command_uses_path_arg() {
local arg="$1"
case "$arg" in
/* | ./* | ../* | ~* | *\\*)
return 0
;;
esac
return 1
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

echo '--- validation helpers ---'
sed -n '296,351p' lib/adapters.sh

echo
echo '--- load_adapter validation chain ---'
sed -n '470,478p' lib/adapters.sh

echo
echo '--- override adapters consuming configured args ---'
sed -n '9,28p' adapters/editor/nano.sh
echo
sed -n '42,62p' adapters/ai/claude.sh

Repository: coderabbitai/git-worktree-runner

Length of output: 2889


🏁 Script executed:

# Check where these validation functions are called
rg '_configured_command_uses_path_arg|_configured_command_has_safe_syntax|_configured_command_is_safe' lib/ adapters/ --no-heading

Repository: coderabbitai/git-worktree-runner

Length of output: 451


🏁 Script executed:

# Test the pattern matching behavior
bash -c '
arg="~/.toolrc"
case "$arg" in
  ~*)
    echo "~/.toolrc matches ~* pattern (would be rejected)"
    ;;
  *)
    echo "~/.toolrc does not match pattern"
    ;;
esac

arg="%s\n"
case "$arg" in
  \;* | \`* | '"'"'\$(\'' | \|* | \&* | '\''>'\'\'* | '\''<'\''*)
    echo "%s\n contains metacharacters (would be rejected)"
    ;;
  *)
    echo "%s\n is safe from _configured_command_has_safe_syntax"
    ;;
esac
'

Repository: coderabbitai/git-worktree-runner

Length of output: 286


🏁 Script executed:

# Test the pattern matching behavior more simply
bash -c '
arg="~/.toolrc"
case "$arg" in
  ~*)
    echo "RESULT: ~/.toolrc matches ~* pattern"
    ;;
  *)
    echo "RESULT: ~/.toolrc does not match pattern"
    ;;
esac
'

bash -c '
arg="%s\n"
case "$arg" in
  *\;* | *\`* | *\|\* | *\&* | *\>* | *\<*)
    echo "RESULT: %s\n contains metacharacters"
    ;;
  *)
    echo "RESULT: %s\n is safe from syntax checks"
    ;;
esac
'

bash -c '
arg="foo & bar"
case "$arg" in
  *\;* | *\`* | *\|\* | *\&* | *\>* | *\<*)
    echo "RESULT: foo & bar contains metacharacters"
    ;;
  *)
    echo "RESULT: foo & bar is safe"
    ;;
esac
'

Repository: coderabbitai/git-worktree-runner

Length of output: 194


Validation is overly restrictive for safe argv-based execution.

With command execution shifted from eval to parsed argv via "$@", literal path-like arguments and raw metacharacters in config values are now safe data, not shell syntax. Yet _configured_command_uses_path_arg() still rejects arguments matching ~* (e.g., ~/.toolrc), and _configured_command_has_safe_syntax() rejects raw metacharacters like & in the unparsed command string. These validations block legitimate configured commands before they reach the override adapters that correctly consume GTR_*_CMD_ARGS separately.

Suggested fix

Remove the path-argument check and pre-parse syntax validation. Keep only command-name validation in _load_adapter():

 _configured_command_is_safe() {
   [ "$#" -gt 0 ] || return 1
 
   local cmd_name="$1"
   shift
 
   case "$cmd_name" in
     */* | *\\*)
       return 1
       ;;
   esac
 
   if _configured_command_is_wrapper "$cmd_name" && [ "$#" -gt 0 ]; then
     return 1
   fi
+
+  return 0
 }
-  if ! _configured_command_has_safe_syntax "$name" \
-    || ! _parse_configured_command parsed_args "$name" \
+  if ! _parse_configured_command parsed_args "$name" \
     || ! _configured_command_is_safe "${parsed_args[@]}"; then

Then remove _configured_command_uses_path_arg() and _configured_command_has_safe_syntax() entirely (lines 296–327).

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/adapters.sh` around lines 296 - 304, Remove the overly restrictive
pre-parse validations: delete the helper functions
_configured_command_uses_path_arg and _configured_command_has_safe_syntax and
stop rejecting arguments like "~/.toolrc" or raw metacharacters, and instead
only validate the adapter command name inside _load_adapter(); ensure
_load_adapter() no longer calls those two functions and only checks the command
name token (leaving GTR_*_CMD_ARGS handling to the adapter overrides) so
argv-style execution via "$@" can accept literal path-like args and
metacharacters as safe data.

Comment on lines +174 to +180
if test "$_gtr_git_common_dir" = ".git"
set -l _gtr_repo_root (git rev-parse --show-toplevel 2>/dev/null)
test -n "$_gtr_repo_root"; or return 1
printf '%s/.gtrconfig\n' "$_gtr_repo_root"
else
printf '%s/.gtrconfig\n' (string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir")
end
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

#!/bin/bash
set -euo pipefail

command -v fish >/dev/null 2>&1 || exit 0

fish <<'FISH'
printf 'current: '
string replace -r '/\\.git$' '' -- '/tmp/repo/.git'
printf '\nfixed:   '
string replace -r '/\.git$' '' -- '/tmp/repo/.git'
printf '\n'
FISH

Repository: coderabbitai/git-worktree-runner

Length of output: 58


🏁 Script executed:

# Check if the file exists and read the relevant lines
cat -n lib/commands/init.sh | sed -n '170,185p'

Repository: coderabbitai/git-worktree-runner

Length of output: 828


🏁 Script executed:

# Also check the surrounding context to understand the purpose better
cat -n lib/commands/init.sh | sed -n '150,190p'

Repository: coderabbitai/git-worktree-runner

Length of output: 1750


🏁 Script executed:

# Search for other occurrences of the regex pattern
rg '/\\\\.git' lib/commands/init.sh

Repository: coderabbitai/git-worktree-runner

Length of output: 164


🏁 Script executed:

# Check git documentation references or comments about linked worktrees
rg -i 'worktree|linked' lib/commands/init.sh | head -20

Repository: coderabbitai/git-worktree-runner

Length of output: 1317


🏁 Script executed:

# Verify the git-common-dir behavior for linked worktrees
# Check if there are any tests or documentation about this
rg -i 'git-common-dir|git.*common.*dir' lib/commands/init.sh -B2 -A2

Repository: coderabbitai/git-worktree-runner

Length of output: 915


🏁 Script executed:

# Check if there's any test coverage for this function
fd -name "*.fish" -o -name "*test*" | xargs grep -l "gtrconfig_path" 2>/dev/null || echo "No test files found"

Repository: coderabbitai/git-worktree-runner

Length of output: 343


Fish worktree path regex is over-escaped and breaks linked worktree config resolution.

In Fish, single-quoted strings keep backslashes literal, so the pattern /\\.git$ matches a literal backslash followed by a dot—not the .git suffix that git rev-parse --git-common-dir actually returns. The shell version of this function correctly uses %/.git parameter expansion, confirming the intent is to strip the /.git suffix. Linked worktrees will fail to find .gtrconfig at the repo root because the suffix is never stripped.

Suggested fix
-    printf '%s/.gtrconfig\n' (string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir")
+    printf '%s/.gtrconfig\n' (string replace -r '/\.git$' '' -- "$_gtr_git_common_dir")
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/commands/init.sh` around lines 174 - 180, The regex passed to string
replace in the branch handling linked worktrees is over-escaped ('/\\.git$') so
Fish treats the backslash literally; change the pattern to '/\.git$' (single
backslash) in the string replace -r call that operates on _gtr_git_common_dir so
the trailing '/.git' suffix is correctly stripped (i.e., update the expression
string replace -r '/\\.git$' '' -- "$_gtr_git_common_dir" to use '/\.git$'
instead).

Comment on lines +135 to +175
_hooks_are_trusted() {
local config_file="$1"
[ ! -f "$config_file" ] && return 0

local trust_path
trust_path=$(_hooks_current_trust_path "$config_file") || return 0 # no hooks = trusted
_hooks_marker_matches_config "$trust_path" "$config_file"
}

# Write a trust marker that matches the reviewed hooks
# Usage: _hooks_write_trust_marker <trust_path> [config_file]
_hooks_write_trust_marker() {
local trust_path="$1"
local config_file="${2:-}"
local temp_path=""
local canonical_config_file=""

mkdir -p "$_GTR_TRUST_DIR" || return 1
canonical_config_file=$(_hooks_canonical_config_path "$config_file") || return 1
temp_path="$(mktemp "$_GTR_TRUST_DIR/.trust.XXXXXX")" || return 1

if ! printf '%s\n' "$canonical_config_file" > "$temp_path"; then
rm -f "$temp_path"
return 1
fi

if ! mv -f "$temp_path" "$trust_path"; then
rm -f "$temp_path"
return 1
fi
}

# Mark .gtrconfig hooks as trusted
# Usage: _hooks_mark_trusted <config_file>
_hooks_mark_trusted() {
local config_file="$1"
local trust_path
trust_path=$(_hooks_current_trust_path "$config_file") || return 0

_hooks_write_trust_marker "$trust_path" "$config_file"
}
Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

Fail closed when trust-key resolution breaks.

_hooks_are_trusted() and _hooks_mark_trusted() both treat _hooks_current_trust_path() failure as success. That collapses “no hooks” and real errors into the same path, so untrusted hooks can run and trust persistence can silently no-op.

Suggested fix
 _hooks_are_trusted() {
   local config_file="$1"
   [ ! -f "$config_file" ] && return 0
 
-  local trust_path
-  trust_path=$(_hooks_current_trust_path "$config_file") || return 0  # no hooks = trusted
+  local hook_content trust_path
+  hook_content=$(_hooks_read_definitions "$config_file") || return 0
+  trust_path=$(_hooks_reviewed_trust_path "$config_file" "$hook_content") || return 1
   _hooks_marker_matches_config "$trust_path" "$config_file"
 }
@@
 _hooks_mark_trusted() {
   local config_file="$1"
-  local trust_path
-  trust_path=$(_hooks_current_trust_path "$config_file") || return 0
+  local hook_content trust_path
+  hook_content=$(_hooks_read_definitions "$config_file") || return 0
+  trust_path=$(_hooks_reviewed_trust_path "$config_file" "$hook_content") || return 1
 
   _hooks_write_trust_marker "$trust_path" "$config_file"
 }
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In `@lib/hooks.sh` around lines 135 - 175, The code currently treats failures from
_hooks_current_trust_path as success (returning 0), merging “no hooks” with real
errors; change the error handling so that when _hooks_current_trust_path fails
it returns non-zero (fail closed). Specifically, in _hooks_are_trusted() replace
the trust_path assignment branch that uses "|| return 0" with "|| return 1" so
resolution errors mark hooks untrusted, and in _hooks_mark_trusted() do the same
(replace "trust_path=$(_hooks_current_trust_path \"$config_file\") || return 0"
with "|| return 1") so marker writes are aborted on resolution errors.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants